add tests and harden command policy - #2

Nice work on the security hardening! The base64 decode check and new dangerous command patterns are valuable additions. Some thoughts:

Security improvements 👍

• extractBase64Candidates catching encoded destructive commands is a smart defensive layer
• find . -delete and git clean -fd patterns fill real gaps

Potential improvements to consider:

1. extractBase64Candidates false positive surface — [A-Za-z0-9+/]{8,}={0,2} matches many benign strings (UUIDs, npm package hashes, URL-safe tokens). In a code-editing context this could flag almost every npm install or curl command. Consider:

   • Only decode if the decoded string is at least 4 chars and contains a shell metachar (|, ;, $, backtick)
   • Or limit candidate length to avoid matching package version hashes

2. rm -rf pattern narrowing — the new regex requires rm -rf / or rm -rf ~ etc., but rm -rf /tmp/test would no longer match (it did before). Was this intentional? The old pattern /\brm\s+-rf\s+\//i was simpler and arguably safer.

3. Test coverage — 2 test cases is a great start, but for a security-critical change I'd suggest adding:

   • A test that benign base64 (like echo SGVsbG8=) is not denied
   • A test for git clean -fdx variant
   • A test for the find / -delete (absolute path) variant

4. Framework choice — Node built-in test runner keeps dependencies minimal, which is great. Just note that it doesn't provide coverage reporting or watch mode, which feat: Add automated test suite and cross-platform CI #19 requires.

Regarding CONTRIBUTING.md overlap — both our PRs update this file. I'm happy to rebase #16 on top of this if it merges first.

Thanks for the security improvements — these are orthogonal to the test infrastructure work in #16 and complement it well.

Thanks for the careful review. I pushed a6ea7bc to address the concrete command-policy items:

• narrowed base64 candidate handling with a max candidate length and decoded-payload screening, so benign encoded text like echo SGVsbG8= is not denied;
• kept destructive decoded payloads blocked, including cm0gLXJmIC8= (rm -rf /);
• broadened destructive path matching so rm -rf /tmp/test is still denied;
• added coverage for find / -mindepth 1 -delete and git clean -fdx;
• kept the existing workspace wipe test for find . -mindepth 1 -delete.

Validation: pnpm check passes locally. Lint still reports the existing warnings only, with no errors.

Agreed on the framework/coverage point: this PR intentionally stays focused on the security hardening and first Node test baseline. The broader coverage/watch-mode/cross-platform matrix work in #16 remains complementary.

Hi @luochen211, thanks for the hardening patch and the additional tests! After reading this PR alongside #16, I have a few suggestions on merge ordering and structure so the two efforts compose cleanly.

Key observation

The test cases in #2 and #16 are complementary, not redundant:

• test: add comprehensive test suite (848 tests, 11 files) #16 covers the API surface, mode behaviors, classic dangerous commands (rm -rf /, shutdown, mkfs, dd, etc.), and per-tool override precedence.
• add tests and harden command policy #2 covers what test: add comprehensive test suite (848 tests, 11 files) #16 does not: encoded-payload bypass, find -delete variants, workspace-wipe variants, and git clean -fd variants.

However, the test infrastructure overlaps. #16 introduces vitest with vitest.config.ts, a GitHub Actions workflow, helpers/fixtures, and a TESTING.md doc. #2 introduces a node:test script, a tsconfig include change, and a root-level tests/ directory. Keeping both runners will fragment CI and contributor mental models.

Recommendation

1. Land test: add comprehensive test suite (848 tests, 11 files) #16 first so the vitest infrastructure is in place.
2. Rebase this PR onto the new main.
3. Drop the following from this PR (superseded by test: add comprehensive test suite (848 tests, 11 files) #16):
   • tests/tool-policy.test.ts (the entire file)
   • the "test" script added to package.json and its wiring into pnpm check
   • the "tests" entry added to tsconfig.json's include
4. Move the six unique edge cases into the packages/core/src/policy/tool-policy.test.ts file introduced by test: add comprehensive test suite (848 tests, 11 files) #16, rewritten in vitest style and using the alias import @step-cli/core/policy/tool-policy.js:
   • denies encoded destructive shell commands
   • allows benign encoded text
   • denies destructive rm paths
   • denies destructive find delete variants
   • denies destructive workspace wipe variants
   • denies git clean forced delete variants
5. Keep packages/core/src/policy/tool-policy.ts hardening — that is the real value of this PR.

End state: hardened policy + comprehensive coverage + your unique edge cases, all on a single test framework. Thanks again for the contribution!

Updated this branch against current main and applied the requested structure change.

What changed:

• removed the standalone tests/tool-policy.test.ts node:test file;
• removed the temporary root tests tsconfig include and node:test package script;
• moved the unique command-policy edge cases into packages/core/src/policy/tool-policy.test.ts in vitest style;
• kept the packages/core/src/policy/tool-policy.ts hardening.

Validation: pnpm test -- packages/core/src/policy/tool-policy.test.ts and full pnpm check both pass locally. The PR now reports mergeable.

Thanks for your first contribution to the project! 🎉